Replace application list view with a Vue ViewModel by martinRenou · Pull Request #402 · simphony/simphony-remote

martinRenou · 2017-04-18T17:02:57Z

No description provided.

stefanoborini · 2017-04-24T15:57:38Z

    // It is only synchronized at initial load.
    var model = new models.ApplicationListModel();
-    var app_list_view = new application_list_view.ApplicationListView(model);
+    var app_list_view = application_list_view.applicationListView;


use underscores.

Also assign the model at this stage.

stefanoborini · 2017-04-24T16:06:22Z

+      We initialize this element with "display: none" to prevent displaying
+      "No applications found" and the loading spinner at the same time
+    -->
+    <li v-show="!loading && model.app_list.length === 0" style="display: none;">


Try to use a v-else.

There's no gain to use v-if and v-else instead of two v-show in terms of lines of code. And my problem is that it changes the behavior of the dom elements, v-ifwill result in rendering the element only if the statement is true, v-showonly changes the style attribute displayof the element, so I have to change the selenium tests which relies on the style attribute

stefanoborini · 2017-04-24T16:06:57Z

+    -->
+    <li v-for="(app, index) in model.app_list"
+        v-bind:class="{ active: index === model.selected_index }"
+        v-on:click="model.selected_index = index"


v-on -> @click

stefanoborini · 2017-04-24T16:07:22Z

+      issues when the javascript is not loaded yet (temporary ?)
+    -->
+    <li v-for="(app, index) in model.app_list"
+        v-bind:class="{ active: index === model.selected_index }"


v-bind also has a shorthand. I think it's just :class

stefanoborini · 2017-04-24T16:10:02Z

+             v-bind:src="get_icon_src(app.app_data)">
+
+        <button class="stop-button"
+                v-show="app.status === 'RUNNING' && index_mouse_over === index"


Try to do this with simple CSS

stefanoborini · 2017-04-24T16:40:29Z

+            stop_application: function(index) {
+                this.stop_application_callback(index);
+            },
+            mouse_enter: function(index) {


maybe as CSS?

stefanoborini · 2017-04-24T16:41:03Z

+                    this.index_mouse_over = index;
+                }
+            },
+            mouse_leave: function() {


maybe with CSS?

stefanoborini · 2017-04-24T16:42:01Z

-        if (self.model.selected_index === index) {
-            self.update_selected();
+        watch: {
+            'model.selected_index': function() { this.selected_app_callback(); }


This will eventually go away when the syncronization is performed through the model

stefanoborini · 2017-04-24T16:51:41Z

                function(val) {  // jshint ignore:line
-                    var widget = configurables[val].view(index);
-                    fieldset.append(widget);
+                    // Vue.js adds an __ob__ property, we have to fix it


If you replace the properties with a list, it should cause no harm and solve this.

stefanoborini · 2017-04-25T08:26:54Z

@@ -1,32 +1,33 @@
 {
-    "name": "simphony-remote-deps",


Use four spaces for indentation. I checked the preferred style guide, and most of the opinions are for four spaces, like in python, including Crockford. We don't have to save space, minification will do that (if ever, js files are cached anyway)

Ok, correction. Google uses two. uhm....

ok, keep two.

Better, keep four for now, and we'll switch to two after this PR is in

stefanoborini · 2017-04-25T08:39:09Z

@@ -1,32 +1,33 @@
 {
-    "name": "simphony-remote-deps",


Better, keep four for now, and we'll switch to two after this PR is in

…property

codecov-io · 2017-04-25T11:24:27Z

Codecov Report

Merging #402 into switch_to_vue will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           switch_to_vue     #402   +/-   ##
==============================================
  Coverage          95.17%   95.17%           
==============================================
  Files                 94       94           
  Lines               3996     3996           
  Branches             256      256           
==============================================
  Hits                3803     3803           
  Misses               139      139           
  Partials              54       54

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c892b79...a8d6dfb. Read the comment docs.

The gain of the last changes was null and it results in test failing. I prefer to undo those changes instead of changing the tests.

good

martinRenou and others added 24 commits April 18, 2017 16:44

Install vue with bower instead of NPM

87df38e

Replace the old ApplicationListView by a Vue ViewModel

d7a09a7

Delete unused handlebars helpers

4c08083

Create ApplicationListView template

14fc736

Fixed imports

051d193

Fix import

b25d692

Started to adapt the controller to the new Application list ViewModel

60ffdfb

Changed data representation of the model

1cbd133

Adapt controller with the new model data representation

b625942

Adapt ApplicationListView with the new model data representation

a9cf5a0

Adapt ApplicationView with the new model data representation

0440ef5

Fixed little issues

31ddfaf

Add comment

27928b2

Add stop button

6760912

Fix linter tests

e3f334e

Fix model tests

c17ef68

Fix view tests

5554651

Try to fix selenium test (DOM elements has been changed)

88b2ca1

Use minified version of Vue

7143e94

Try to fix selenium tests

adee7bd

Fix selenium tests

c5891a3

Fix selenium test

7c82057

Fix selenium tests

08c22a7

Fixed init path

ed7ae38

martinRenou requested a review from stefanoborini April 25, 2017 08:27

stefanoborini reviewed Apr 25, 2017

View reviewed changes

stefanoborini previously requested changes Apr 25, 2017

View reviewed changes

martinRenou added 3 commits April 25, 2017 09:42

Fix indentation of the bower file

3acffe7

Now assign the model to the ApplicationListView at instantiation

d4c982c

Use shorthands for vue attributes

a15b9d3

martinRenou added 6 commits April 25, 2017 10:55

The stop-button hover event is now triggered with pure CSS

5d5ccd9

Clear unused code

93a48cd

Use filters for icon and application name

48eb4a8

Configurables is now a list, fixing the issue of Vue adding a __ob__ …

0c35794

…property

Try to fix focus selenium test

11eaf35

Fix jstests

3f52280

martinRenou added 3 commits April 25, 2017 12:34

Use if else-if

88e398f

Undo last commit

1bea069

The gain of the last changes was null and it results in test failing. I prefer to undo those changes instead of changing the tests.

Fix issue with unknown "this"

a8d6dfb

martinRenou merged commit 4e5c077 into switch_to_vue Apr 25, 2017

martinRenou changed the title ~~Replace application list view with a Vue ViewModel [WIP]~~ Replace application list view with a Vue ViewModel Apr 25, 2017

martinRenou deleted the replace_application_list2 branch April 25, 2017 13:45

Uh oh!

Conversation

martinRenou commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Apr 25, 2017 •

edited

Loading